Ensure consistent Connection header value #179
Conversation
Depending on how you start a connection, the `Connection` header may be unexpectedly set to `close`
Creating a new instance of `Net::HTTP` and then calling its `request` method (or any of its methods that call `request`, eg: `get`) will first set the `Connection` header to `close` (if it's not already set), then start the connection, and re-call itself
Where as initiating a request with class methods, or by manually starting a connection before calling `request` will _not_ set the `Connection` header
As the default connection in HTTP 1.1 is `keep-alive`, having the `Connection` header silently set to `close` is unexpected
Examples:
```rb
uri = URI('https://example.com/test')
http = Net::HTTP.new(uri.host, uri.port)
http.use_ssl = true
req = Net::HTTP::Get.new(uri.path)
req['connection'] # => nil
res = http.request(req)
req['connection'] # => 'close'
http = Net::HTTP.new(uri.host, uri.port)
http.use_ssl = true
http.start
req = Net::HTTP::Get.new(uri.path)
req['connection'] # => nil
res = http.request(req)
req['connection'] # => nil
```
From what I can see this functionality was added 24 years ago when implicit starts to GET and HEAD requests were introduced. Not long later the shortcut class methods (eg: `Net::HTTP.get`) were added, which called `start` as part of the method body, bypassing the implicit start therefore not setting the `Connection: close` header, creating inconsistent behaviour
This PR stops the `Connection` header from being set to `close` when calling `request` without an open connection, as that matches the behaviour of the more commonly used class methods, as well as allowing the HTTP 1.1 default of `keep-alive` to be used unless the `Connection` header is explicitly set
|
I believe the current behavior is by design. If you would like to pipeline requests, you should wrap them inside a Net::HTTP.start(uri.host, uri.port) do |http|
req = Net::HTTP::Get.new(uri.path)
req['connection'] # => nil
res = http.request(req)
req['connection'] # => 'close'
endIt sounds like you may want the connection/file descriptor left open even without explicit connection management via def (Net::HTTPRequest).new(...)
req = super
req['connection'] = 'keep-alive'
req
end |
|
@jeremyevans thanks for taking the time to review this. I must admit it hadn't occurred to me this was by design. I can see how it makes sense to use connection=>close when you're just making a single request to a server, rather than forcing the user to manually close a keep-alive connection We weren't trying to deliberately leave connections hanging, the opposite actually. It was some legacy code I was working on that was using net/http in the connection=>close way, combined with a server that freaked out if the connection wasn't keep alive. I probably should have included the |
Depending on how you start a connection, the
Connectionheader may be unexpectedly set tocloseCreating a new instance of
Net::HTTPand then calling itsrequestmethod (or any of its methods that callrequest, eg:get) will first set theConnectionheader toclose(if it's not already set), then start the connection, and re-call itselfWhere as initiating a request with class methods, or by manually starting a connection before calling
requestwill not set theConnectionheaderAs the default connection in HTTP 1.1 is
keep-alive, having theConnectionheader silently set tocloseis unexpectedExamples:
From what I can see this functionality was added 24 years ago when implicit starts to GET and HEAD requests were introduced. Not long later the shortcut class methods (eg:
Net::HTTP.get) were added, which calledstartas part of the method body, bypassing the implicit start therefore not setting theConnection: closeheader, creating inconsistent behaviourThis PR stops the
Connectionheader from being set toclosewhen callingrequestwithout an open connection, as that matches the behaviour of the more commonly used class methods, as well as allowing the HTTP 1.1 default ofkeep-aliveto be used unless theConnectionheader is explicitly set